Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#ISSUE 1005] - Expand checkstyle to all modules (root) #1059

Closed
wants to merge 22 commits into from

Conversation

damonxue
Copy link

@damonxue damonxue commented Sep 4, 2023

fixes #1005

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement
I had signed an [Individual Contributor License Agreement] in Apache ShenYu project.

@lprimak
Copy link
Contributor

lprimak commented Sep 4, 2023

there are still compiler errors :(

@damonxue

This comment was marked as off-topic.

@lprimak
Copy link
Contributor

lprimak commented Sep 5, 2023

yea... the code needs to compile has nothing to do with checkstyle. Also owasp is only secure checks, not the code style etc.

@damonxue
Copy link
Author

damonxue commented Sep 5, 2023

yea... the code needs to compile has nothing to do with checkstyle. Also owasp is only secure checks, not the code style etc.

It's my fault, i did not see the build plugins. And did the compile error cause by the property of root.dir?

@lprimak
Copy link
Contributor

lprimak commented Sep 5, 2023

No problem. The point of this issue is to fix Shiro code so it complies with the checkstyle rules and passes.

No, root.dir doesn't cause any issues.

@damonxue

This comment was marked as outdated.

@lprimak
Copy link
Contributor

lprimak commented Sep 6, 2023

I ran into some backlog in my day job. Please be patient.

@lprimak
Copy link
Contributor

lprimak commented Sep 9, 2023

First of all, great job, thanks!
I have fixed up the configuration for the most part to the way it was.

However, some major and minor issues remain
Some minor issues:

  • Line endings. Looks like you have windows line endings in your source code. This repo uses unix line endings. Can you fix that?

Major issues:

  • Tests and demos need to be included in checkstyle checking
  • There are too many changes in checkstyle configuration. Most things should be done inline using @SuppressWarnings and not in configuration

@lprimak
Copy link
Contributor

lprimak commented Sep 9, 2023

I have started the process to fix code with the new config. Ready for you to take over and finish it. The things I did should get you started.

Thank you once again, great job!

@lprimak lprimak self-assigned this Sep 9, 2023
@lprimak lprimak added pending-cla shiro-2.0.0 java Pull requests that update Java code core Core Modules labels Sep 9, 2023
@lprimak
Copy link
Contributor

lprimak commented Sep 9, 2023

The only configuration that should be modified now is by adding more JavadocPackages in suppressins.xml
Everything else should be made inline unless I missed something egregious

@lprimak lprimak marked this pull request as draft September 9, 2023 18:48
@damonxue
Copy link
Author

Ok, i see. I will done the unfinished.

@damonxue damonxue reopened this Sep 10, 2023
@damonxue damonxue marked this pull request as ready for review September 10, 2023 23:17
@damonxue
Copy link
Author

@lprimak @bmarwell pls review.

@lprimak
Copy link
Contributor

lprimak commented Sep 11, 2023

You are doing a way too much...

  • Only code that fails checkstyle should be reformatted, no more no less.
  • Line length should not be ignored / suppressed, but fixed. Looks like you added many suppressions for that. There should be none.

Once again I appreciate your effort, great job!
I believe this is gettin closer!

@damonxue
Copy link
Author

  • The reason for formatting too many files is to make sure that they all end with 'lf'.
  • I will change too many 'lineLength'.

@damonxue
Copy link
Author

damonxue commented Sep 12, 2023

to #1069

@damonxue damonxue closed this Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core Modules java Pull requests that update Java code pending-cla shiro-2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand checkstyle to all modules (root)
2 participants